feature: switch AWS Endpoints for European Souvereign Cloud #11356
Conversation
📝 WalkthroughWalkthroughEndpoint construction now selects a domain suffix dynamically: standard ".amazonaws.com", China ".amazonaws.com.cn" for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/aws/flb_aws_util.c (2)
86-93: Region-to-suffix mapping correctly implements EU Sovereign Cloud support.The logic properly routes:
- China regions (cn-north-1, cn-northwest-1) →
.amazonaws.com.cn- EU Sovereign Cloud (eusc-de-east-1) →
.amazonaws.eu- All other regions →
.amazonaws.com(default)💡 Optional: Consider a lookup table for future scalability
If additional EU Sovereign Cloud regions are added in the future, a lookup table or pattern-based approach could improve maintainability:
+static const char* get_domain_suffix(const char* region) { + if (strcmp(region, "cn-north-1") == 0 || strcmp(region, "cn-northwest-1") == 0) { + return AWS_SERVICE_ENDPOINT_SUFFIX_COM_CN; + } + if (strncmp(region, "eusc-", 5) == 0) { /* Match all eusc-* regions */ + return AWS_SERVICE_ENDPOINT_SUFFIX_EU; + } + return AWS_SERVICE_ENDPOINT_SUFFIX_COM; +}However, the current hardcoded approach is acceptable for the known regions.
107-112: Consider checking for snprintf truncation.The current error handling only checks for encoding errors (
bytes < 0). While truncation shouldn't occur due to the correct length calculation, defensive coding would also verify thatbytes < lento catch any unexpected truncation.🛡️ More robust snprintf error handling
- bytes = snprintf(endpoint, len, AWS_SERVICE_ENDPOINT_FORMAT, service, region, domain_suffix); - if (bytes < 0) { + bytes = snprintf(endpoint, len, AWS_SERVICE_ENDPOINT_FORMAT, service, region, domain_suffix); + if (bytes < 0 || bytes >= len) { flb_errno(); flb_free(endpoint); return NULL; }This guards against both encoding errors and unexpected truncation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
🧰 Additional context used
🧬 Code graph analysis (2)
src/aws/flb_aws_util.c (1)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
tests/internal/aws_util.c (2)
src/aws/flb_aws_util.c (1)
flb_aws_endpoint(78-116)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (3)
src/aws/flb_aws_util.c (2)
36-39: LGTM! Clear parameterization of endpoint format.The new constants cleanly separate the endpoint format from domain suffixes, enabling support for standard (.com), China (.com.cn), and EU Sovereign Cloud (.eu) regions.
95-99: LGTM! Length calculation is accurate.The calculation correctly accounts for all components in the format
"%s.%s%s":
- Service name length
- Dot separator (1 byte)
- Region name length
- Domain suffix length
- Null terminator (1 byte)
tests/internal/aws_util.c (1)
166-172: LGTM! Test case properly validates EU Sovereign Cloud endpoint.The test correctly verifies that region
eusc-de-east-1produces the endpointcloudwatch.eusc-de-east-1.amazonaws.eu, directly addressing the issue described in #1057. Memory management and test structure follow the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/aws/flb_aws_util.c (1)
91-93: Consider prefix-based matching for EU Sovereign Cloud regions.The current implementation correctly handles the
eusc-de-east-1region as specified in the issue. However, if AWS introduces additional EU Sovereign Cloud regions (which would likely follow theeusc-*naming pattern), each would require a code change.♻️ Proposed refactor for scalability
- else if (strcmp("eusc-de-east-1", region) == 0) { + else if (strncmp(region, "eusc-", 5) == 0) { domain_suffix = AWS_SERVICE_ENDPOINT_SUFFIX_EU; }This approach would automatically support any future EU Sovereign Cloud regions without requiring code changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
🧰 Additional context used
🧬 Code graph analysis (2)
src/aws/flb_aws_util.c (1)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
tests/internal/aws_util.c (2)
src/aws/flb_aws_util.c (1)
flb_aws_endpoint(78-116)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (7)
src/aws/flb_aws_util.c (6)
36-39: LGTM! Clean constant definitions for domain suffixes.The introduction of separate domain suffix constants and the updated format string enables flexible endpoint construction for different AWS partitions (standard, China, EU Sovereign Cloud).
76-77: LGTM! Comment accurately reflects the updated endpoint format.
87-90: LGTM! China region handling is correct and complete.The code properly handles both China regions (cn-north-1 and cn-northwest-1) by using the
.amazonaws.com.cndomain suffix.
95-99: LGTM! Length calculation is correct and clear.The calculation properly accounts for all endpoint components (service, dot separator, region, domain suffix, and null terminator).
101-105: LGTM! Memory allocation and error handling are correct.
107-112: LGTM! Endpoint construction and error handling are correct.The
snprintfcall properly formats the endpoint using the selected domain suffix, and error handling ensures memory is freed on failure.tests/internal/aws_util.c (1)
166-171: LGTM! Test coverage for EU Sovereign Cloud endpoint is excellent.The new test case properly validates that the
eusc-de-east-1region generates the correct endpoint with the.amazonaws.eudomain suffix, as required by issue #1057. The test follows the established pattern and includes proper memory management.
| #define AWS_SERVICE_ENDPOINT_FORMAT "%s.%s%s" | ||
| #define AWS_SERVICE_ENDPOINT_SUFFIX_COM ".amazonaws.com" | ||
| #define AWS_SERVICE_ENDPOINT_SUFFIX_COM_CN ".amazonaws.com.cn" | ||
| #define AWS_SERVICE_ENDPOINT_SUFFIX_EU ".amazonaws.eu" |
There was a problem hiding this comment.
This could be corresponds to this press release:
https://aws.amazon.com/blogs/security/introducing-the-overview-of-the-aws-european-sovereign-cloud-whitepaper/
But this type of URL like here:
https://github.com/fluent/fluent-bit/pull/11356/changes#diff-c3e141a0b55a2488f85e10509c2556b996b97425d47d1b5e75ccf736d0203b1bR169
cloudwatch.eusc-de-east-1.amazonaws.eu couldn't be published yet, right?
There was a problem hiding this comment.
EuSC is available as preview and will soon be publicly launched as well. logs.eusc-de-east-1.amazonaws.eu does exist
|
We need to rebase off the current master. So, could you remove merge commit and forcely push rebased your branch? |
kabakakao
requested review from
celalettin1286,
edsiper,
niedbalski and
patrick-stephens
as code owners
Signed-off-by: Stephan <[email protected]>
Signed-off-by: Stephan <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/aws/flb_aws_util.c (1)
75-77: Minor documentation clarity suggestion.The comment notation
[com(.cn)|eu]is functional but could be made clearer. Consider:/* - * https://service.region.amazonaws.[com(.cn)|eu] + * https://service.region.amazonaws.{com|com.cn|eu} */This explicitly shows the three possible domain suffixes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/aws/flb_aws_util.ctests/internal/aws_util.c
🧰 Additional context used
🧬 Code graph analysis (2)
tests/internal/aws_util.c (2)
src/aws/flb_aws_util.c (1)
flb_aws_endpoint(78-116)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/aws/flb_aws_util.c (1)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)
🔇 Additional comments (5)
tests/internal/aws_util.c (1)
166-171: LGTM! Test case correctly validates EU Sovereign Cloud endpoint.The test properly verifies that
eusc-de-east-1region produces the expected.amazonaws.eudomain suffix, directly addressing the issue described in #1057. Memory management is consistent with existing tests.Consider adding an additional test case for another
eusc-*region (e.g.,eusc-de-west-1) to verify the prefix-based matching works for any EU Sovereign Cloud region, not just the specific one reported in the issue.src/aws/flb_aws_util.c (4)
36-39: LGTM! Well-structured constants for endpoint construction.The domain suffix constants are clearly named, include the leading dot appropriately, and the format string correctly represents the endpoint structure
service.region{suffix}.
86-93: LGTM! Region detection logic is correct.The implementation properly handles:
- China regions with exact string matching (only two known regions)
- EU Sovereign Cloud with prefix matching (
eusc-) for flexibility with future regions- Default
.amazonaws.comfor all other regionsThe use of
strncmpwith length 5 for theeusc-prefix is appropriate.
95-99: LGTM! Length calculation is correct.The buffer size calculation properly accounts for:
- Service name length
- Dot separator (1 byte)
- Region name length
- Domain suffix length (dynamically sized)
- Null terminator (1 byte)
107-108: LGTM! Endpoint construction and error handling are correct.The
snprintfcall correctly formats the endpoint asservice.region{domain_suffix}, and the error handling properly checks for both negative return values and potential truncation (bytes >= len).
|
@cosmo0920 I have rebased from your master and now my fork reports as just 2 ahead of your master, and neither of those are merge commits, both are signed. Does this achieve what you want to? |
|
So the gist is when you specify an EU region it should use EU endpoints I believe. We should update the docs to make this clear as well. Probably a philosophical question but should UK regions target EU or US endpoints? |
|
Our commit linter complains like that: We need to follow the linting rule. |
Not all EU regions should use these endpoints, only EU regions that are designated as a "European Sovereign Cloud" region (currently there is only one). These regions have a unique prefix ( |
| len += 3; | ||
| is_cn = FLB_TRUE; | ||
| /* China regions end with amazonaws.com.cn */ | ||
| if (strcmp("cn-north-1", region) == 0 || |
There was a problem hiding this comment.
Outside scope of this PR, but we should probably change these to just check for the cn- prefix instead of the full region name, similar to how you've done eusc below.
Interestingly there is a UK "iso" partition, which uses the |
4 participants
Use European Souvereign Cloud specific Endpoints if neccessary
Fixes: aws/aws-for-fluent-bit#1057
Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.